Skip to content

fix: ensure that the parent of a SourceRoot cannot be itself #17381

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Jun 10, 2024
Merged

fix: ensure that the parent of a SourceRoot cannot be itself #17381

merged 1 commit into from
Jun 10, 2024

Conversation

roife
Copy link
Member

@roife roife commented Jun 10, 2024 •
edited
Loading

fix #17378.

In FileSetConfig.map, different roots might be mapped to the same root_id due to deduplication in ProjectFolders::new:

// Example from rustup
/Users/roife/code/rustup/target/debug/build/rustup-863a063426b56c51/out
/Users/roife/code/rustup

In source_root_parent_map, r-a might encounter paths where their SourceRootId (i.e. root_id) is identical, yet one the them is the parent of the another. This situation can cause the root_id to be its own parent, potentially leading to an infinite loop.

This PR resolves such cases by adding a check.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 10, 2024
@roife roife force-pushed the fix-issue-17378 branch from 963486e to 733995c Compare June 10, 2024 15:16
@roife
Copy link
Member Author

roife commented Jun 10, 2024 •
edited
Loading

Could a circular reference occur here?

For example:

rootA: ["/ROOT/a", "/ROOT/a/b/c/d"]
rootB: ["/ROOT/a/b"]

This might cause rootA to be the parent of rootB while rootB is the parent of rootA.🤯

Or perhaps for paths with a parent-child relationship within the same SourceRoot, we only consider the root path (which means r-a should exit the inner loop when root_id == root2_id).

@rami3l
Copy link
Member

rami3l commented Jun 10, 2024

@roife I can confirm that your test build on 73399 works for me. Nice work!

@Veykril
Copy link
Member

Veykril commented Jun 10, 2024

We already depend on petgraph so we could just use a proper graph impl for this instead that then deals with accidental cycle handling if they can actually occur. Though this entire thing is very hacky in the first place...

Thanks a lot for investigating!
@bors r+

@bors
Copy link
Contributor

bors commented Jun 10, 2024

📌 Commit 733995c has been approved by Veykril

It is now in the queue for this repository.

@Veykril
Copy link
Member

Veykril commented Jun 10, 2024 •
edited
Loading

@lnicola we might wanna do a second release after this as today's release becomes mostly unusable for a bunch of people (and then there is also the random startup panics that I also (presumably) fixed this morning). If you have the time that is this week :)

@bors
Copy link
Contributor

bors commented Jun 10, 2024

⌛ Testing commit 733995c with merge c86d623...

@bors
Copy link
Contributor

bors commented Jun 10, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing c86d623 to master...

@bors bors merged commit c86d623 into rust-lang:master Jun 10, 2024
11 checks passed
@davidhewitt davidhewitt mentioned this pull request Jun 12, 2024
Closed
@philips-software-forest-releaser philips-software-forest-releaser bot mentioned this pull request Jun 18, 2024
Merged
@philips-software-forest-releaser philips-software-forest-releaser bot mentioned this pull request Jun 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Version 0.3.1992 rust-analyzer process spins out of control in macOS VSCode
5 participants
@roife @rami3l @Veykril @bors @rustbot